fix(packages): implement multiple fixes to new components #43
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements multiple fixes to new components based on code review feedback. The changes focus on improving code quality, maintainability, and UI/UX consistency across various components.
- Refactored duplicate mobile detection logic and extracted common constants
- Enhanced UI styling for better responsive design and accessibility
- Improved code structure and formatting consistency
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| PreviewModal.tsx | Refactored mobile detection, improved key generation, and enhanced responsive button layout |
| PreviewModal.stories.tsx | Updated story examples with better placeholder icons and realistic data |
| Total.tsx, BTCFeeAmount.tsx, BBNFeeAmount.tsx | Replaced hardcoded decimal places with constants and improved code readability |
| FeesSection.tsx | Removed unnecessary blank line |
| FeeItem.tsx | Reorganized conditional rendering logic |
| AmountSubsection.tsx | Added input styling to hide number input spinners |
| constants.ts | Added new constants for decimal places |
| FinalityProviderItem.tsx | Enhanced layout with flex shrink classes and spacing |
| SubSection/index.ts | Removed unused export |
| CounterButton.tsx | Improved styling and layout logic |
| CounterButton.stories.tsx | Removed redundant story |
| import { PropsWithChildren, ReactNode } from "react"; | ||
| import { twMerge } from "tailwind-merge"; | ||
|
|
||
| const toKebabCase = (str: string): string => { |
There was a problem hiding this comment.
The toKebabCase function should be moved to a utilities file rather than being defined inline in this component, as it could be reused elsewhere in the codebase.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@jonybur this comment make sense to me
| Attention! | ||
| </Heading> | ||
| <Text variant="body2" className="text-secondary"> | ||
| 1. No third party possesses your staked BTC. You are the only one who can unbond and withdraw your stake. |
There was a problem hiding this comment.
[nitpick] The numbered text content should be extracted to constants or props to improve maintainability and potential internationalization support.
Copilot uses AI. Check for mistakes.
| })() | ||
| : total; | ||
| export function Total({ total, coinSymbol, hint, title = "Total", className, decimals = BTC_DECIMAL_PLACES }: TotalProps) { | ||
| let formattedTotal; |
There was a problem hiding this comment.
The variable formattedTotal should be declared with an explicit type annotation (string) for better type safety and code clarity.
| let formattedTotal; | |
| let formattedTotal: string; |
Copilot uses AI. Check for mistakes.
| @@ -0,0 +1,2 @@ | |||
| export const BTC_DECIMAL_PLACES = 8; | |||
| export const BBN_DECIMAL_PLACES = 5; No newline at end of file | |||
There was a problem hiding this comment.
There is trailing whitespace at the end of the line that should be removed.
| export const BBN_DECIMAL_PLACES = 5; | |
| export const BBN_DECIMAL_PLACES = 5; |
Copilot uses AI. Check for mistakes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/babylon-core-ui/src/widgets/sections/PreviewModal/PreviewModal.tsx
Fixed
Show fixed
Hide fixed
packages/babylon-core-ui/src/widgets/sections/PreviewModal/PreviewModal.tsx
Fixed
Show fixed
Hide fixed
| counter: number; | ||
| max: number; | ||
| onAdd: () => void; | ||
| alwaysShowCounter?: boolean; |
There was a problem hiding this comment.
this is no longer needed, right?
There was a problem hiding this comment.
Will address this refactor in a follow up PR
| return <Avatar url={bsnLogoUrl} alt={bsnName} variant="rounded" size="tiny" className="mr-1" />; | ||
| } | ||
|
|
||
| const placeholderLetter = bsnName?.charAt(0).toUpperCase() || "?"; |
There was a problem hiding this comment.
bsnName is not an optional field. You don't need this ? thing here right?
There was a problem hiding this comment.
Will address this refactor in a follow up PR
| ); | ||
| }; | ||
|
|
||
| const shortenAddress = (value: string): string => { |
There was a problem hiding this comment.
This method isn’t specific to this component. It should be abstracted and moved to a utility module for common use.
There was a problem hiding this comment.
Will address this refactor in a follow up PR
|
|
||
| return isMobile; | ||
| } | ||
| const WINDOW_BREAKPOINT = 640; |
There was a problem hiding this comment.
this should be moved to a centralised place too. sounds like a constant value which you can use across the codebase
There was a problem hiding this comment.
Will address this refactor in a follow up PR
| <div className="flex flex-col gap-3"> | ||
| {bsns.map((bsnItem, index) => ( | ||
| <div key={`bsn-${index}`} className="flex w-full items-center justify-center gap-2 py-1"> | ||
| <div key={`bsn-${toKebabCase(bsnItem.name)}-${index}`} className="flex w-full items-center justify-center gap-2 py-1"> |
There was a problem hiding this comment.
why not use the bsn id?
There was a problem hiding this comment.
Will address this refactor in a follow up PR
Revises all comments from @gbarkhatov
babylonlabs-io/core-ui#136
Allows for counter to display 1/1